Skip to content

refactor(robot): move fknm/frne C++ extensions under robot/#526

Merged
petercorke merged 10 commits into
mainfrom
refactor/c-extensions
Jul 3, 2026
Merged

refactor(robot): move fknm/frne C++ extensions under robot/#526
petercorke merged 10 commits into
mainfrom
refactor/c-extensions

Conversation

@petercorke

Copy link
Copy Markdown
Owner

Summary

  • Complete Phase 3 cleanup: delete legacy CPython-API sources (fknm.cpp, fknm.h, frne.c), superseded by the nanobind port
  • Move fknm.py/frne.py facades from top-level roboticstoolbox/ into robot/ — they're robot-kinematics/dynamics internals, not general package-level modules
  • Rename+move the C++/C source dir core/robot/cpp-extensions/, update CMakeLists.txt paths (build-config only — never a Python package; compiled _fknm_c/_frne_c still install to the top-level roboticstoolbox package)
  • Fix the resulting circular import: moving fknm.py under robot/ made tools/p_servo.py's module-level import force robot/__init__.py to run before tools/__init__.py finished. Fixed by deferring the import inside angle_axis(), and having Dynamics.py/DHRobot.py pull rtb_get_param/rtb_set_param directly from roboticstoolbox.tools.params instead of the top-level package
  • Flesh out robot/cpp-extensions/README.md: synopsis, lazy-serialization/@_dirties_fknm/@_dirties_frne pattern, corrected column-major note, updated file table
  • Document the lazy-import workaround as tech debt in tech-debt.md (proper fix: move p_servo.py into robot/ itself)

Test plan

  • pip install -e . --no-build-isolation rebuilds cleanly against the new CMakeLists.txt paths
  • Full local suite: 656 passed, 9 skipped, 0 failed (macOS)
  • CI matrix (ubuntu/windows/macos × Python 3.10–3.13) — this PR is specifically to get Windows validated: the vendored Eigen headers were already close to Windows' MAX_PATH limit (hence the existing "shorten TEMP path" CI step), and this move adds ~16 chars to every path under the C++ source tree

🤖 Generated with Claude Code

petercorke and others added 10 commits June 30, 2026 18:15
Rename C extensions fknm→_fknm_c and frne→_frne_c (CMakeLists.txt,
PyModuleDef names, PyInit_* functions).  Add pure-Python facade modules
fknm.py and frne.py that try to import the C extensions and provide
Python fallbacks for the ETS evaluation functions when C is absent
(Pyodide, CI without a C toolchain).  Symbolic inputs are detected once
in the facade; callers make unconditional calls with no try/except.

- fknm.py: Python fallbacks for fkine, jacob0, jacobe, hessian0,
  hessiane; IK C-wrappers raise RuntimeError when C absent; ET_T raises
  TypeError to trigger existing fallback in ET.A(); Robot_link_T is a
  no-op (visualisation only)
- frne.py: thin wrapper; init() returns None when C absent so rne()
  dispatches to rne_python()
- ETS.py: removed all try/except blocks from eval/jacob0/jacobe/
  hessian0/hessiane; now single-line facade calls with _data= and _n=
  kwargs for the Python path
- DHRobot.py: rne() checks _rne_ob is None before dispatching to C
- hessian0/hessiane also check _is_symbolic(J0/Je) not just _is_symbolic(q)
- tests/test_fknm_fallback.py: 41 tests; context managers use new=_py_func
  pattern; timing tests run with C active

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- use a dict to map string name to int
- name the parameter axis_type rather than joint_type
…e renames

- Drop UserList in favour of MutableSequence; implement the five required
  abstract methods and delegate to self._data (plain list).
- Add _dirties_fknm decorator; marks _fknm_stale on any mutation so the C
  handle is rebuilt lazily on first use rather than eagerly on every change.
- _copy_to_cpp replaces the old _building context-manager pattern.
- Add __repr__ and __eq__ to BaseETS (MutableSequence provides neither).
- ETS/ETS2 insert() signature now matches MutableSequence: (index, value).
  append() is inherited and works via insert(len, value).
- split() wraps each segment in self.__class__() instead of returning raw slices.
- ETS/ETS2 __init__ raises TypeError for unrecognised arg types (was silent).
- ET.__axis_to_number: use dict.get(axis, 0) to handle SE3 axis gracefully.
- DHRobot: _rne_ob → _frne, _init_rne → _copy_to_cpp, _dynchanged → _frne_stale;
  lazy rebuild at top of rne() removes the old @_check_rne decorator.
- Link/DHLink: _listen_dyn → _dirties_frne; wrapper renamed to match.
- Update test_insert in test_ETS.py and test_ETS2.py for new insert API.

657/660 tests pass; 3 pre-existing URDF/xacro failures unrelated to this change.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rewrites both C extension glue layers to use nanobind instead of the old
CPython API, eliminating manual reference counting and PyCapsule memory
management.

Key changes in fknm_nb.cpp:
- EigenRef4d (dynamic-stride Ref) for 4×4 matrix inputs (base, tool, Tep):
  accepts C-contiguous, F-contiguous, or any-stride numpy arrays without
  Python-side layout conversion
- EigenRefJd for 6×n Jacobian inputs: fixes hessian corruption when J0
  comes from DHRobot.jacob0() (C-contiguous) vs ETS.jacob0() (F-contiguous)
- dcarray (nb::c_contig) for q vectors; nanobind enforces stride-1 access
- Stride fix: nanobind ndarray strides are in elements, not bytes; corrected
  make_2d_F strides from {sizeof(double), n*sizeof(double)} to {1, n}
- ET_update: bool params (nanobind rejects Python bool where int expected)

Key changes in frne_nb.cpp:
- RobotObj wrapper with proper RAII destructor (no PyCapsule null-destructor)

Key changes in fknm.py facade:
- ET_init/ET_update catch TypeError for symbolic (object-dtype) inputs
- _to_q_array: np.ascontiguousarray for q (handles list/tuple/non-contiguous)
- _to_f_array: np.asarray for SE3 → ndarray (Eigen::Ref handles layout)
- _ik_args_se3: SE3 → ndarray conversion only (no contiguity enforcement)

Key change in DHRobot.rne():
- np.ascontiguousarray(q/qd/qdd) before frne loop; trajectory slices from
  np.c_[...].T are non-contiguous, causing wrong results with stride-1 C code

All 558 non-GUI tests pass.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…import

Complete Phase 3 cleanup: delete the legacy CPython-API sources
(fknm.cpp, fknm.h, frne.c) superseded by the nanobind port, and
relocate what remains to reflect that fknm/frne are robot
kinematics/dynamics internals, not general package-level modules:

- move fknm.py/frne.py facades from top-level roboticstoolbox/
  into robot/
- rename+move the C++/C source dir core/ -> robot/cpp-extensions/
  and update CMakeLists.txt paths (never a Python package, so this
  is build-config only; compiled _fknm_c/_frne_c still install to
  the top-level roboticstoolbox package)
- flesh out robot/cpp-extensions/README.md: synopsis, how the
  lazy-serialization/@_dirties_fknm/@_dirties_frne pattern works,
  corrected column-major gotcha, updated file table

Moving fknm.py under robot/ made tools/p_servo.py's module-level
import of Angle_Axis force robot/__init__.py to run before
tools/__init__.py finished initializing, since roboticstoolbox/
loads tools before robot and robot depends on tools at module
scope. Fix: defer the import inside angle_axis() (lazy import),
and have Dynamics.py/DHRobot.py pull rtb_get_param/rtb_set_param
directly from roboticstoolbox.tools.params instead of the
top-level package, decoupling from init order.

Document the lazy-import workaround as tech debt (proper fix is
moving p_servo.py into robot/ itself).

656 tests passing, 0 failed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TEMPORARY, for CI validation only — revert before merging. rtb-data
hasn't been republished to PyPI since data files were added to main
(publishing now would break existing installs pinned to a version
that doesn't expect these changes). Points at the git subdirectory
instead so CI can validate against current data without a real
release.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@petercorke petercorke merged commit ecb5879 into main Jul 3, 2026
1 of 13 checks passed
@petercorke petercorke deleted the refactor/c-extensions branch July 3, 2026 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant